Add JSON Support #83
Add JSON Support #83
Conversation
lsmith77
force-pushed
the
json-support
branch
2 times, most recently
from
c2f4eff to
1d6b29d
Compare
Codecov Report
@@ Coverage Diff @@
## master #83 +/- ##
=============================================
- Coverage 98.31% 82.06% -16.26%
- Complexity 47 56 +9
=============================================
Files 3 4 +1
Lines 119 145 +26
=============================================
+ Hits 117 119 +2
- Misses 2 26 +24
Continue to review full report at Codecov.
|
|
@nonoesp any feedback here? thx |
|
Hey, @lsmith77 — I'll try to take a look at this soon (and hopefully test it). Thanks for the work! |
|
Hi, @lsmith77 - I have little time to review and test this. I'd really appreciate if you could describe the steps I should follow to test this pull request and verify everything works? 🙏 |
|
I stilll owe this PR tests. but fundamentally you have to make sure |
|
I would appreciate advice on how to best test this. f.e. should I use |
|
I am also wondering if instead or in addition to the command, there should be a migration? |
| $versionedHiddenFields = $this->versionedHiddenFields ?? []; | ||
| $this->makeVisible($versionedHiddenFields); | ||
| $version->model_data = serialize($this->attributesToArray()); | ||
| $version->model_data = $this->getEncoding() ? json_encode($this->attributesToArray()) : serialize($this->attributesToArray()); |
There was a problem hiding this comment.
Hey! Shouldn't this be $version->model_data = $this->getEncoding() == 'json' ? json_encode($this->attributesToArray()) : serialize($this->attributesToArray());?
There was a problem hiding this comment.
indeed! .. goes to show that this PR needs tests
There was a problem hiding this comment.
Yes, it does! I was testing manually yesterday and it doesn't seem to quite work with my site.
|
@nonoesp I think I fixed the bug you encountered, however still didn't have time to figure out how to add tests and generate an efficient migration script. hopefully this week I will manage to do it. |
lsmith77
force-pushed
the
json-support
branch
3 times, most recently
from
8c29cd1 to
599fa0c
Compare
|
@nonoesp I hope this is now ready to be merged. I added a very simple test case and also documented the process to create a migration with lazy loading (which however requires laravel 8). |
|
Hi, @lsmith77 - I'll be taking a look at this! |
|
ping? |
|
Thanks for the reminder! |
|
Hi @lsmith77 can you resolve the conflicts? |
lsmith77
force-pushed
the
json-support
branch
from
599fa0c to
99ae136
Compare
|
done |
fix for #82
note I am not so expierienced with developing for laravel .. it seems to work ok on a quick check.
I need to add tests. aside from this there are some "taste" things to decide .. like using the ternary syntax inside the Version class or if the call to config() should be "cached" in case multiple versionable models are handled in one request.